Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stream: improving error msg for stream #8801

Closed
wants to merge 4 commits into from
Closed

stream: improving error msg for stream #8801

wants to merge 4 commits into from

Conversation

italoacasas
Copy link
Contributor

@italoacasas italoacasas commented Sep 27, 2016 •
edited
Loading

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
  • stream
Description of change
  • Improving error msg when Transform._transform() is not implement
  • Improving error msg when Readable._read() is not implement
  • Removing extra word for error msg when Writable._write() is not implemented

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Sep 27, 2016
@Trott Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 27, 2016
@Trott
Copy link
Member

Trott commented Sep 27, 2016

I think we're still in a condition where (unfortunately) changing an error message means the change is semver major. (/cc @jasnell in case I'm wrong) Labeling semver-major.

Change looks good to me, although I would probably be inclined to omit the word method since the () makes it clear that it's a function in the first place. But that's totally a nit you can ignore.

@ronkorving
Copy link
Contributor

ronkorving commented Sep 27, 2016 •
edited
Loading

I don't care too much about "method" being there or not, but I'm happy with this change 👍 (and agree it's semver-major)

@italoacasas
Copy link
Contributor Author

italoacasas commented Sep 27, 2016 •
edited
Loading

@Trott my first through was _transform() is not implemented but then I see this on _writable https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L435, I just trying to maintain consistency. Maybe the correct solution is to fix _writable instead of adding method here.

@lpinca
Copy link
Member

lpinca commented Sep 27, 2016

+1 for using consistent error messages. It's semver major so changing _write error message could also be an option.

@italoacasas italoacasas changed the title stream: improving msg when transform._transform() method is not implemented stream: improving error msg for stream Sep 27, 2016
@italoacasas
Copy link
Contributor Author

italoacasas commented Sep 27, 2016 •
edited
Loading

I just remove method from Writable._write() and add the same error msg in Readable._read()

Fishrock123
Fishrock123 reviewed Sep 27, 2016
View reviewed changes
.gitignore

# Visual Studio Code
.vscode/
jsconfig.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please add a newline at the end here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually want to add that data to .gitignore? If I'm not wrong there is an ongoing discussion in another PR for .idea and another open PR to whitelist stuff instead of blacklisting.
I think .idea, .vscode and the others should be in the user global gitignore not here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the PR: #6963

... I think is just better to add this kind of common files to .gitignore that asking people to add this to the global ignore. Just because this way we don't need to mention again something about this topic and really this don't hurt anyone

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@italoacasas #8016 is a much better option imho.

@jasnell
Copy link
Member

jasnell commented Sep 27, 2016

LGTM with nits addressed and green CI.

cjihrig
cjihrig requested changes Sep 27, 2016
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM without the .gitignore changes.

.gitignore
@@ -93,3 +92,7 @@ test.tap
# Xcode workspaces and project folders
*.xcodeproj
*.xcworkspace

# Visual Studio Code
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be done elsewhere.

@italoacasas
Copy link
Contributor Author

Done

@lpinca
Copy link
Member

lpinca commented Sep 27, 2016

LGTM

imyller
imyller approved these changes Sep 29, 2016
View reviewed changes
Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@imyller imyller self-assigned this Sep 29, 2016
@imyller
Copy link
Member

imyller commented Sep 29, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/4321/

@imyller
Copy link
Member

imyller commented Sep 29, 2016

I'll start landing this:

  • Five LGTMs
  • No objections
  • Requested modifications have been made
  • CI tests passed

imyller pushed a commit that referenced this pull request Sep 29, 2016
@italoacasas @imyller
stream: improve stream error messages
560a589 
Improve message when tranform._transform() method is not implemented
Improve error message when Readable._read() is not implemented
Remove extra word in err msg when Writable._write() when not implemented
Remove extra word in err msg when Transform._transform() when not implemented

PR-URL: #8801
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@imyller
Copy link
Member

imyller commented Sep 29, 2016

Landed in 560a589

Thank you for your effort and contribution, @italoacasas

@imyller imyller closed this Sep 29, 2016
@imyller imyller removed their assignment Sep 29, 2016
@jasnell
Copy link
Member

jasnell commented Oct 10, 2016

@nodejs/ctc ... any objections to this landing in v7.x?

jasnell pushed a commit that referenced this pull request Oct 10, 2016
@italoacasas @jasnell
stream: improve stream error messages
2f05af4 
Improve message when tranform._transform() method is not implemented
Improve error message when Readable._read() is not implemented
Remove extra word in err msg when Writable._write() when not implemented
Remove extra word in err msg when Transform._transform() when not implemented

PR-URL: #8801
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fishrock123 Fishrock123 Fishrock123 left review comments

@cjihrig cjihrig cjihrig approved these changes

@imyller imyller imyller approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@italoacasas @Trott @ronkorving @lpinca @jasnell @imyller @Fishrock123 @cjihrig @nodejs-github-bot